Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revamp widget connector #391

Merged
merged 48 commits into from
Jul 10, 2014
Merged

Revamp widget connector #391

merged 48 commits into from
Jul 10, 2014

Conversation

westonruter
Copy link
Contributor

Resolves (in part) #308

This adds support for tracking widget changes in the customizer, improves the logic for tracking changes on the widgets admin page, and allows tracking of widget changes no matter how they are made (e.g. via WP-CLI).

New features:

  • Log changes to widgets in the customizer
  • Handle batch operations on widgets
  • Add deactivated/reactivated actions
  • Add basic support for old single widgets
  • Remove hooks which tie into the UI, and rely solely on actions which fire when the underlying data is modified.
  • Note: sidebar Stream meta has been renamed to sidebar_id to correspond with widget_id. No migration script to rename stream to stream_id has been included.

Todo:

  • Improve behavior of logging movement of widgets between sidebars when in the customizer.
  • Should widget_id ever be added as a context?
  • When moving a widget from one sidebar to another, should there be one Stream record with two contexts, or two records with one context each?
  • Add database migrations for any changed stream meta (e.g. sidebar => sidebar_id)
  • Add widget edit link, in addition to the widget area edit link. (See Add widget edit link in addition to the widget area edit link #521)

Argument was passed to wp_stream_post_insert_error action
Still have work to do to make this work properly in Customizer
Use a different string template mechanism which prevents args from being needlessly added to Stream meta
Fix label for movement to reverse from/to sidebars
Use new tpl vars for summaries
Conflicts:
	connectors/widgets.php
} else {
// Neither a name nor a title are available, so use the sidebar ID
$message = __( '{widget_id} widget updated', 'stream' );
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fjarrett this may have undone what you did in 8818976

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter Yeah it would be ideal to put the sidebar name in these summaries again, also we should use _x() to give context to the translator.

Can you help me understand the differences between $widget_id_base, $widget_id and $name? I'm a little confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The $name is the kind of widget that it is, e.g. Text, RSS, Categories, etc. Ideally you'd want to see both the name and the title in the stream.

If the $widget_id is text-123 then the $widget_id_base is text, so just the ID without the number attached to it.

Conflicts:
	connectors/widgets.php
 * Point to expirimental PHPCS and WPCS for rulset subsets
 * Introduce config file for managing Travis before_script environment
 * Remove phpcs.ruleset.xml since we're referencing a core ruleset

See xwp/wp-dev-lib#30
Conflicts:
	bin/.travis.yml
	includes/admin.php
	phpcs.ruleset.xml
@frankiejarrett
Copy link
Contributor

Closing this PR until more progress can be made in the near future.

@westonruter
Copy link
Contributor Author

What was outstanding?

@frankiejarrett
Copy link
Contributor

global $wp_registered_sidebars;

if ( array_key_exists( $sidebar, $wp_registered_sidebars ) ) {
$links[ __( 'Edit Widget Area', 'stream' ) ] = admin_url( 'widgets.php#' . $sidebar ); // xss ok (@todo fix WPCS rule)
}
// @todo Also old_sidebar_id and new_sidebar_id
// @todo Add Edit Widget link
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are optional nice to haves. Future enhancements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So then I guess #521 should be reopened as well.

frankiejarrett added a commit that referenced this pull request Jul 10, 2014
@frankiejarrett frankiejarrett merged commit 4476e22 into develop Jul 10, 2014
@frankiejarrett frankiejarrett deleted the widget-customizer branch July 10, 2014 15:58
@frankiejarrett
Copy link
Contributor

/five @westonruter!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants